Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Koblitz-based and Keccak256-based custom witness verification #3209

Merged
merged 29 commits into from
May 10, 2024

Conversation

AnnaShaleva
Copy link
Member

@AnnaShaleva AnnaShaleva commented May 3, 2024

Description

This PR is a port of nspcc-dev/neo-go#3425 and is an alternative to #3205 that does not require significant core protocol changes. This PR doesn't require a hard-fork. In this PR:

  • Extend native CryptoLib's verifyWithECDsa by supporting Keccak256 hasher;
  • Add example of how to build a custom sig/multisig witness for Koblitz signatures and Keccak256 hasher and test it.
  • Move custom witness builder from test to Contract.cs class (if needed, we need to decide about it). Some CreateKoblitzSigRedeemScript and CreateKoblitzMultiSigRedeemScript APIs may be added to this class. Discussed, not needed.
  • Implement CalculateNetworkFee extension that allows to properly calculate network fee for custom transaction witnesses. Ref. Improve CalculateNetworkFee for contract signers #2805 (in progress). Won't be included into 3.7, ref. Neo v3.7.0 plan #2905 (comment).
  • Add more unit-tests for native CryptoLib's verifyWithECDsa.

Rational

This PR exists because of #3205 (comment).

A note for reviewers

The main builder logic is located in the tests/Neo.UnitTests/SmartContract/Native/UT_CryptoLib.cs. Please, review this file carefully. We need to decide whether these builders have to be moved outside of the tests to some Contract.cs class. If yes, then I'll update the PR.

Simple signature builder is in its final state. Multisignature builder requires minor simplification (ref. nspcc-dev/neo-go#3425 (review) and nspcc-dev/neo-go#3425 (comment)), but it works and is also almost done. I'll update the PR once we resolve the original conversations.

Also, this PR changes CryptoLib's manifest (name of the verifyWithECDsa argument was changed), and thus requires node resync from the genesis. But it's a minor incompatibility that won't affect anything. Anyway, we are going to resync mainnet/testnet for 3.7.

A special request for review from @vang1ong7ang, you'll like it.

Results

Single signature verification

What user signs Invocation script length Verification script length Witness verification cost*
keccak256([4-bytes-network-magic-LE, txHash-bytes-BE]) 66 bytes 110 bytes 2154270 GAS

* Verification cost is based on the default execution fee value.
** See the script variations in nspcc-dev/neo-go#3425, this PR contains the best one from our POW.

Multisignature verification

What user signs Multisig M out of N Invocation script length Verification script length Witness verification cost*
keccak256([4-bytes-network-magic-LE, txHash-bytes-BE]) 3 out of 4 198 bytes (= 66*3 bytes) 264 bytes 8390070 GAS

* Verification cost is based on the default execution fee value.
** See the script variations in nspcc-dev/neo-go#3425, this PR contains the best one from our POW.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Unit tests for verifyWithECDsa CryptoLib's API;
  • TestVerifyWithECDsa_CustomTxWitness_SingleSig;
  • TestVerifyWithECDsa_CustomTxWitness_MultiSig.

Checklist:

src/Neo/Cryptography/Crypto.cs Outdated Show resolved Hide resolved
src/Neo/Cryptography/Crypto.cs Outdated Show resolved Hide resolved
src/Neo/Cryptography/Crypto.cs Show resolved Hide resolved
src/Neo/Cryptography/Hasher.cs Show resolved Hide resolved
src/Neo/Cryptography/Crypto.cs Show resolved Hide resolved
src/Neo/Cryptography/Crypto.cs Outdated Show resolved Hide resolved
src/Neo/Cryptography/Crypto.cs Show resolved Hide resolved
src/Neo/Cryptography/Crypto.cs Show resolved Hide resolved
src/Neo/Cryptography/Crypto.cs Outdated Show resolved Hide resolved
src/Neo/Cryptography/Crypto.cs Outdated Show resolved Hide resolved
src/Neo/Cryptography/Crypto.cs Show resolved Hide resolved
src/Neo/Cryptography/Crypto.cs Outdated Show resolved Hide resolved
src/Neo/Cryptography/Crypto.cs Outdated Show resolved Hide resolved
src/Neo/Cryptography/Hasher.cs Show resolved Hide resolved
@Jim8y
Copy link
Contributor

Jim8y commented May 4, 2024

@AnnaShaleva really like your comment style, very very detailed explaination on everything. Easy to follow.

AnnaShaleva added a commit that referenced this pull request May 4, 2024
IsMultiSigContract should return proper true/false result even if some
garbage is provided as an input. Without this commit an exception is
possible during IsMultiSigContract execution during subsequent public
key decoding from compressed form:
```
  Failed TestIsMultiSigContract [16 ms]
  Error Message:
   Test method Neo.UnitTests.SmartContract.UT_Helper.TestIsMultiSigContract threw exception:
System.FormatException: Invalid point encoding 221
  Stack Trace:
      at Neo.Cryptography.ECC.ECPoint.DecodePoint(ReadOnlySpan`1 encoded, ECCurve curve) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/Cryptography/ECC/ECPoint.cs:line 98
   at Neo.SmartContract.Helper.IsMultiSigContract(ReadOnlySpan`1 script, Int32& m, Int32& n, List`1 points) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/SmartContract/Helper.cs:line 189
   at Neo.SmartContract.Helper.IsMultiSigContract(ReadOnlySpan`1 script) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/SmartContract/Helper.cs:line 123
   at Neo.UnitTests.SmartContract.UT_Helper.TestIsMultiSigContract() in /home/anna/Documents/GitProjects/neo-project/neo/tests/Neo.UnitTests/SmartContract/UT_Helper.cs:line 65
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
```

Note that this change is compatible wrt the callers' behaviour. We need
this change to properly handle non-Secp256r1-based witness scripts, ref.
#3209.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
shargon pushed a commit that referenced this pull request May 5, 2024
)

IsMultiSigContract should return proper true/false result even if some
garbage is provided as an input. Without this commit an exception is
possible during IsMultiSigContract execution during subsequent public
key decoding from compressed form:
```
  Failed TestIsMultiSigContract [16 ms]
  Error Message:
   Test method Neo.UnitTests.SmartContract.UT_Helper.TestIsMultiSigContract threw exception:
System.FormatException: Invalid point encoding 221
  Stack Trace:
      at Neo.Cryptography.ECC.ECPoint.DecodePoint(ReadOnlySpan`1 encoded, ECCurve curve) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/Cryptography/ECC/ECPoint.cs:line 98
   at Neo.SmartContract.Helper.IsMultiSigContract(ReadOnlySpan`1 script, Int32& m, Int32& n, List`1 points) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/SmartContract/Helper.cs:line 189
   at Neo.SmartContract.Helper.IsMultiSigContract(ReadOnlySpan`1 script) in /home/anna/Documents/GitProjects/neo-project/neo/src/Neo/SmartContract/Helper.cs:line 123
   at Neo.UnitTests.SmartContract.UT_Helper.TestIsMultiSigContract() in /home/anna/Documents/GitProjects/neo-project/neo/tests/Neo.UnitTests/SmartContract/UT_Helper.cs:line 65
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
```

Note that this change is compatible wrt the callers' behaviour. We need
this change to properly handle non-Secp256r1-based witness scripts, ref.
#3209.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
A port of
nspcc-dev/neo-go@1e2b438.

This commit contains minor protocol extension needed for custom
Koblitz-based verification scripts (an alternative to
#3205).

Replace native CryptoLib's verifyWithECDsa `curve` parameter by
`curveHash` parameter which is a enum over supported pairs of named
curves and hash functions. NamedCurve enum mark as deprecated and
replaced by NamedCurveHash with compatible behaviour.

Even though this change is a compatible extension of the protocol, it
changes the genesis state due to parameter renaming (CryptoLib's
manifest is changed). But we're going to resync chain in 3.7 release
anyway, so it's not a big deal.

Also, we need to check mainnet and testnet compatibility in case if
anyone has ever called verifyWithECDsa with 24 or 25 `curve` value.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Group the set of common operations required to emit
System.Contract.Call appcall.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Koblitz-based and Keccak-based transaction witness verification for
single signature and multisignature ported from
nspcc-dev/neo-go#3425.

An alternative to #3205.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
1. Make prologue be exactly the same as regular CheckMultisig.
2. But instead of "SYSCALL System.Crypto.CheckMultisig" do INITSLOT and K check.
3. This makes all of the code from INITSLOT below be independent of N/M, so
   one can parse the script beginning in the same way CheckMultisig is parsed and
   then just compare the rest of it with some known-good blob.
4. The script becomes a tiny bit larger now, but properties above are too good.

Ported from
nspcc-dev/neo-go@34ee294.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Make the script a bit shorter. ABORTMSG would cost a bit more.

Ported from
nspcc-dev/neo-go@fb16891.
Ref.
nspcc-dev/neo-go#3425 (comment).

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
All flag is too wide. A port of
nspcc-dev/neo-go@fe292f3.

Ref.
nspcc-dev/neo-go#3425 (comment).

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
src/Neo/Cryptography/Crypto.cs Show resolved Hide resolved
/// <returns><see langword="true"/> if the signature is valid; otherwise, <see langword="false"/>.</returns>
public static bool VerifySignature(ReadOnlySpan<byte> message, ReadOnlySpan<byte> signature, ReadOnlySpan<byte> pubkey, ECC.ECCurve curve)
public static bool VerifySignature(ReadOnlySpan<byte> message, ReadOnlySpan<byte> signature, ReadOnlySpan<byte> pubkey, ECC.ECCurve curve, Hasher hasher = Hasher.SHA256)
Copy link
Member

@cschuchardt88 cschuchardt88 May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think this should be marked as [Obsolete] and add creating of new method. We dont want to break wallets, dapps, etc.

Copy link
Member Author

@AnnaShaleva AnnaShaleva May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behaviour is compatible. It was SHA256 before this change, and it's now SHA256 by default. Users don't have to change anything in their code, and nothing is broken by this change.

AnnaShaleva added a commit to neo-project/neo-devpack-dotnet that referenced this pull request May 6, 2024
It will be replaced by NamedCurveHash in the next commit.

A part of neo-project/neo#3209.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
AnnaShaleva added a commit to neo-project/neo-devpack-dotnet that referenced this pull request May 6, 2024
Mark VerifyWithECDsa method as obsolete.

Ref. native CryptoLib update from neo-project/neo#3209.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

// The resulting witness verification cost is 2154270 * 10e-8GAS.
// The resulting witness Invocation script (66 bytes length):
// NEO-VM > loadbase64 DEARoaaEjM/3VulrBDUod7eiZgWQS2iXIM0+I24iyJYmffhosZoQjfnnRymF/7+FaBPb9qvQwxLLSVo9ROlrdFdC
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jim8y, this must not be changed, it’s an automatic output.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert, please.

@AnnaShaleva
Copy link
Member Author

Please make the test I asked for testing Transaction.VerifyWitness

@cschuchardt88, all tests are already included. TestVerifyWithECDsa_CustomTxWitness_SingleSig and TestVerifyWithECDsa_CustomTxWitness_MultiSig do call VerifyStateDependent under the hood. And VerifyStateDependent calls Transaction.VerifyWitness.

@superboyiii
Copy link
Member

Thanks for your explaination! @AnnaShaleva
I just add witness scope to ensure it could pass checkwitnessinternal of GAS, everything works well now!

        /// <summary>
        /// Process "test k1" command
        /// </summary>
        [ConsoleCommand("test k1", Category = "Wallet Commands")]
        private void OnTestK1Command()
        {
            byte[] privatekey = "f011e762cbaa7d9596ed97921a7653ceb66e852d9bffeed5a620a24bd8b0c147".HexToBytes(); ;
            string publichexK1 = "04ff3df16f92c2a6ebde21fbaa499417fe8c2d5108bcf708e5f1f7a4218c0dcec0b842ad407495ecd90137adeb3d784a89951aba65eb2a57a8c3d8c712e259ce81";
            ECPoint publickeyK1 = ECPoint.Parse(publichexK1, ECCurve.Secp256k1);
            using ScriptBuilder vrf = new();
            vrf.EmitPush((byte)NamedCurveHash.secp256k1Keccak256); // push Koblitz curve identifier and Keccak256 hasher.
            vrf.Emit(OpCode.SWAP); // swap curve identifier with the signature.
            vrf.EmitPush(publickeyK1.EncodePoint(true)); // emit the caller's public key.

            // Construct and push the signed message. The signed message is effectively the network-dependent transaction hash,
            // i.e. msg = [4-network-magic-bytes-LE, tx-hash-BE]
            // Firstly, retrieve network magic (it's uint32 wrapped into BigInteger and represented as Integer stackitem on stack).
            vrf.EmitSysCall(ApplicationEngine.System_Runtime_GetNetwork); // push network magic (Integer stackitem), can have 0-5 bytes length serialized.

            // Convert network magic to 4-bytes-length LE byte array representation.
            vrf.EmitPush(0x100000000); // push 0x100000000.
            vrf.Emit(OpCode.ADD, // the result is some new number that is 5 bytes at least when serialized, but first 4 bytes are intact network value (LE).
                    OpCode.PUSH4, OpCode.LEFT); // cut the first 4 bytes out of a number that is at least 5 bytes long, the result is 4-bytes-length LE network representation.

            // Retrieve executing transaction hash.
            vrf.EmitSysCall(ApplicationEngine.System_Runtime_GetScriptContainer); // push the script container (executing transaction, actually).
            vrf.Emit(OpCode.PUSH0, OpCode.PICKITEM); // pick 0-th transaction item (the transaction hash).

            // Concatenate network magic and transaction hash.
            vrf.Emit(OpCode.CAT); // this instruction will convert network magic to bytes using BigInteger rules of conversion.

            // Continue construction of 'verifyWithECDsa' call.
            vrf.Emit(OpCode.PUSH4, OpCode.PACK); // pack arguments for 'verifyWithECDsa' call.
            EmitAppCallNoArgs(vrf, CryptoLib.CryptoLib.Hash, "verifyWithECDsa", CallFlags.None); // emit the call to 'verifyWithECDsa' itself.

            // Account is a hash of verification script.
            var vrfScript = vrf.ToArray();
            var acc = vrfScript.ToScriptHash();
            ConsoleHelper.Info("K1 address:\n", $"{acc}");
            ScriptBuilder sb = new ScriptBuilder();
            sb.EmitDynamicCall(NativeContract.GAS.Hash, "transfer", acc, UInt160.Parse("0x10c823717e6c50782dd11c50e91945f6c1f51dce"), 100000000, "test");
            var snapshot = NeoSystem.StoreView;
            Random rand = new();
            var tx = new Transaction
            {
                Attributes = [],
                NetworkFee = 1_00_0000,
                Nonce = (uint)rand.Next(),
                Script = sb.ToArray(),
                ValidUntilBlock = NativeContract.Ledger.CurrentIndex(snapshot) + NeoSystem.Settings.MaxValidUntilBlockIncrement,

                Signers = [new Signer { Account = acc, Scopes = WitnessScope.CalledByEntry }],
                SystemFee = 0,
                Version = 0,
                Witnesses = []
            };

            using (ApplicationEngine engine = ApplicationEngine.Run(sb.ToArray(), snapshot.CreateSnapshot(), tx, settings: NeoSystem.Settings, gas: ApplicationEngine.TestModeGas))
            {
                if (engine.State == VMState.FAULT)
                {
                    throw new InvalidOperationException($"Failed execution for '{Convert.ToBase64String(sb.ToArray())}'", engine.FaultException);
                }
                tx.SystemFee = engine.GasConsumed;
            }

            var tx_signature = Crypto.Sign(tx.GetSignData(NeoSystem.Settings.Network), privatekey, ECCurve.Secp256k1, Hasher.Keccak256);

            // inv is a builder of witness invocation script corresponding to the public key.
            using ScriptBuilder inv = new();
            inv.EmitPush(tx_signature); // push signature.

            tx.Witnesses =
            [
                new Witness { InvocationScript = inv.ToArray(), VerificationScript = vrfScript }
            ];

            if (!ConsoleHelper.ReadUserInput("Relay tx? (no|yes)").IsYes())
            {
                ConsoleHelper.Info("txraw:\n", $"{Convert.ToBase64String(tx.ToArray())}");
                return;
            }
            NeoSystem.Blockchain.Tell(tx);
            ConsoleHelper.Info("Signed and relayed transaction with hash:\n", $"{tx.Hash}");
        }
    }

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to merge for me

Copy link
Member

@superboyiii superboyiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for me as well.

@cschuchardt88
Copy link
Member

cschuchardt88 commented May 9, 2024

Still think the name NamedCurveHash enum is misleading. It a prefix identifier. NamedCurvePrefix would allow us to add other identifiers to this list in the future. Because you having something like this NamedCurveHash.secp256k1MutliSigurnatureKeccak256 instead of something more like.

NamedCurvePrefix.Keccak256
NamedCurvePrefix.MutliSigurnature
NamedCurvePrefix.secp256k1
NamedCurvePrefix.secp256r1
NamedCurvePrefix.Sha256
NamedCurvePrefix.RedeemSigurature

At least here you can mix and match and can ensure the values will always be different; not the same.

Also I think price for verification should be re-calulated and adjust it.

@superboyiii
Copy link
Member

Merge?

@NGDAdmin NGDAdmin merged commit ef11769 into master May 10, 2024
6 checks passed
@NGDAdmin NGDAdmin deleted the custom-witness branch May 10, 2024 02:36
AnnaShaleva added a commit to neo-project/neo-modules that referenced this pull request May 10, 2024
Fetch the updated master (d713d2bc30ed22fd20620fd84ab72b58d6b93dd6) and
ensure that everything works correctly after the merge of
neo-project/neo#3209.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
shargon pushed a commit to neo-project/neo-modules that referenced this pull request May 10, 2024
Fetch the updated master (d713d2bc30ed22fd20620fd84ab72b58d6b93dd6) and
ensure that everything works correctly after the merge of
neo-project/neo#3209.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
@roman-khimov roman-khimov added this to the v3.7.0 milestone May 10, 2024
AnnaShaleva added a commit to neo-project/neo-devpack-dotnet that referenced this pull request May 10, 2024
It will be replaced by NamedCurveHash in the next commit.

A part of neo-project/neo#3209.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
AnnaShaleva added a commit to neo-project/neo-devpack-dotnet that referenced this pull request May 10, 2024
Mark VerifyWithECDsa method as obsolete.

Ref. native CryptoLib update from neo-project/neo#3209.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Jim8y pushed a commit to neo-project/neo-devpack-dotnet that referenced this pull request May 11, 2024
…rveHash` argument (#1035)

* Native: mark NamedCurve enum as deprecated

It will be replaced by NamedCurveHash in the next commit.

A part of neo-project/neo#3209.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

* Native: replace NamedCurve enum with NamedCurveHash enum

Mark VerifyWithECDsa method as obsolete.

Ref. native CryptoLib update from neo-project/neo#3209.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

* Native: update NamedCurveHash values for Keccak256 hasher

Use 122 and 123 respectively for secp256k1Keccak256 and
secp256r1Keccak256.

Port the
neo-project/neo@e7d9122.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

* neo: fetch 3.7.1 version of the neo core

Released commit 1d957922a1aec90d0ec852402dc6eca496841ed4.

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>

---------

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request May 13, 2024
Use 122 and 123 respectively for Secp256k1Keccak256 and
Secp256r1Keccak256, ref.
neo-project/neo#3209 (comment).

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this pull request May 13, 2024
Use 122 and 123 respectively for Secp256k1Keccak256 and
Secp256r1Keccak256, ref.
neo-project/neo#3209 (comment).

Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants